@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/296/pipeline

This allows the default behaviour in node to follow the intention of js

This is also not true and was not the intention - browsers error on unhandled rejections too. One of the things we've been getting since adding the unhandledRejection hook is constant positive feedback for logging as well as negative one for not crashing.

It would have been great to discuss things like that in the promises working group. Since this is completely backwards incompatible I am strongly -1. It is not an option to remove the regular way how people deal with this.

AFAIK it is also not possible to mirror the current implementation in userland even with the events exposed.

I believe it is best to keep these things in core so everyone takes advantage of it by default. Opt-out instead of opt-in.

By hte way: By the same logic we should deprecate uncaughtException, stop exiting or logging on errors and expose an async_hook for exiting.

i can add the promiseResolve back as deprecated if you want? i assumed it wasn't needed because async_hooks is experimental still...

as to the argument about how promises are supposed to work: they are explicitly intended to be able to silently fail. what browser consoles do is completely irrelevant because that's a debug facility. (we could probably do the same thing with our inspector, which would be also fine)

@BridgeAR

AFAIK it is also not possible to mirror the current implementation in userland even with the events exposed.

i tried to design this with that in mind (rejectWithNoHandler includes the rejection reason, etc) and using process.nextTick will make it identical (except for storing asyncId instead of promise)

@benjamingr browsers provide a console warning on unhandled rejections, which they then retroactively erase if it becomes handled - node doesn't have that capability.

they are explicitly intended to be able to silently fail.

I do not believe this is the case nor does the spec make any such requirement. That said: Under all proposed implementations promises are able to silently fail - just like you can ignore synchronous errors:

process.on('uncaughtException', e => { /* console.log('https://i.imgur.com/c4jt321.png') */ });

You can ignore unhandled rejections:

process.on('uncaughtException', e => { /* console.log('https://i.imgur.com/c4jt321.png') */ });

Which will continue to work forever under all proposed changes. The question is about what the default behaviour should be.

We have a consensus at this point in Node.js that ignoring uncaught errors should not be the default behaviour.

what browser consoles do is completely irrelevant because that's a debug facility. this is all for debugging

Again, the same argument could be made for Node.js not exiting on uncaught errors. Since a significant part of our user-base use async/await it means that in practice they don't distinguish between unhandled rejections and uncaught errors in terms of how they handle errors anyway.

@ljharb

@benjamingr browsers provide a console warning on unhandled rejections, which they then retroactively erase if it becomes handled - node doesn't have that capability.

The browser error handling model is completely different though - Node.js crashes on uncaught errors (by default) whereas browsers "chug on". Both behaviours have been this way for well over 8 years.

I was just pointing out browsers consider unhandled rejections as an error and log based on it. Node.js provides the "erasing" by providing the "rejectionHandled" hook which I haven't actually seen ever used but it's possible and was added in the same PR.

I do not believe this is the case nor does the spec make any such requirement.

the spec says An implementation of HostPromiseRejectionTracker must complete normally in all cases.. i would argue that killing the process is not a normal completion, but i don't really want to get into those semantics.

aside from that, unhandledRejection (with the emit on gc proposal) can't guarantee that it will be fired for the things it should be unless someone is using global.gc, which kind of undermines the debugging potential of it. By not making these (arguably confusing for that reason) apis, we can let people find the best behaviour for what they're trying to do, similar to how people often use tools like winston even though node has console.

these changes also mean that in the common case, nothing is slowed down by promise state tracking.

@devsnek I encourage you to read all the discussion above as well as the nodejs/promises repo discussions about this issue.

The gist is:

• We can't really know when a promise rejection will never be handled (halting problem)
• We can warn when a promise is handled asynchronously (and not within a microtask) since that has been very rare in practice. We've added this and got tremendous positive community feedback.
• GC is the only tool we have for detecting most true positives but no false positives at all. Note this has false negatives like a unhandled rejection the app keeps a global reference for. Also note this cannot be implemented in user-land since it relies on core GC behaviour.
• Since GC isn't perfect - when a promise isn't handled synchronously there is a high true positive chance - so it makes sense to still log these.
• We let users choose the behaviour they want in all cases. This is only about the default behaviour.

In a gist the correct behaviour we settled on was:

• If you can prove (GC) an exception is unhandled do the same thing we do for uncaught exceptions - exit the process.
• If you think with a high probability that a rejection is unhandled - show that as a warning since that's what most users find beneficial (we have a recent Netflix survey that also shows this). This is easy to opt out of.

It's not perfect - but sadly I think it's the best we might be able to do. You are encouraged to read the background discussions and participate in the promises-debugging team which still has an open "call for members". We'd love for more people to get involved.

the spec says An implementation of HostPromiseRejectionTracker must complete normally in all cases.. i would argue that killing the process is not a normal completion, but i don't really want to get into those semantics.

That's not HostPromiseRejectionTracker (which just tracks any rejections) - we are strictly talking about code that happens after that when the promise gets GCd. At this point control has been yielded to the platform and only platform code is running. An implementation may do as it pleases. This is entirely Node.js's domain and not ECMAScript's.

Also note this cannot be implemented in user-land since it relies on core GC behaviour.

the destroy hook lets us do that right?

in any case, wasn't the whole point here to catch all the bad things happening? if we don't catch them we aren't solving the problem (with regard to gc) and having that as a default is imo confusing and misleading (not to mention that having all these promise state trackers by default is annoying)

@benjamingr @BridgeAR unhandledRejection event remains. This PR still provides a solution to exposing the events requested in nodejs/promises-debugging#8 (and other fun stuff)

i still strongly recommend against node defining what an unhandled rejection is in any case but that's a conversation for another issue/pr

CI https://ci.nodejs.org/job/node-test-pull-request/16319/

I've signed off both PR (this and #22218), but we should really reconcile them before landing either one. See @jasnell comment #22218 (comment).

@BridgeAR Same question as in the other PR: Can you explain what you'd like the TSC to decide or do with this at the meeting? (Or should we remove it from the agenda?)

EDIT: Are you asking the TSC to decide whether we should go with this approach or #22218? Is it unlikely that you and @devsnek or others will come to an agreement on that?

If I've understood the conversation above, there are currently 3 objecting Collaborators on this PR: @benjamingr, @BridgeAR, and @AndreasMadsen. If I've misunderstood and any of you don't object to this at this time, please comment! Thanks!

Adding blocked here and in #22218 pending decision about which approach to go with.

@nodejs/tsc PTAL.

@benjamingr your review doesn't show up anymore but i assume it is outdated.

Thanks everybody for putting so much work into this! Would somebody be willing to summarize #22218 and #21857 so it's easier to understand the pros and cons of the two approaches?

@devsnek This would at least need a rebase?

that's more than I'm willing to do at this point.